ci: reduce PR test runtime without reducing coverage#294
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe changes add Rust CI caching, restore normal nextest scheduling, stabilize daemon fixture shutdown and process ownership tests, and route recipe generation coverage through the ChangesCI performance changes
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The Redis fixture can still hit the ten-second supervisor fallback when a client connection is active during shutdown.
Reviewed changes — This PR restores parallel nextest scheduling, removes redundant CI work, and repairs slow process fixtures while retaining application and recipe CLI coverage.
- Restore normal test scheduling — Deletes the blanket
daemon-lifecyclegroup so independent daemon tests can run concurrently. - Streamline the macOS CI job — Adds the pinned
Swatinem/rust-cacheaction and intentionally removes the internal rustdoc and standalone recipe command gates. - Preserve recipe CLI coverage — Runs
generate-recipe-fixturesandgenerate-manifestthrough the compiledpv-releasebinary inside the existing integration test. - Repair managed-resource fixtures — Directly supervises the fast-exit Python Mailpit fixture and delegates Redis and RustFS shutdown away from their
serve_forever()threads. - Characterize script ownership — Adds macOS integration coverage for ownership verification of directly executable Python shebang scripts.
- Document the performance design — Records the diagnosis, constraints, implementation sequence, and validation evidence.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — The new commit closes the Redis fixture shutdown gap identified in the prior review and adds direct regression coverage for the active-client case.
- Daemonized Redis fixture handlers — Set
RedisServer.daemon_threadsso a handler blocked on an open client connection cannot keep the fixture process alive after shutdown. - Covered active-client shutdown — Added
redis_fixture_stops_gracefully_with_active_client_connectionto retain a verified Redis client connection while asserting supervised shutdown avoids the ten-second fallback.
GPT Sol ｜ 𝕏
There was a problem hiding this comment.
Important
The latest commit removes the only regression coverage for the active-client Redis shutdown failure fixed after the prior review.
Reviewed changes — Reviewed the test-only delta since the prior Pullfrog review, which removed the Redis active-client shutdown regression test while retaining the fixture implementation fix.
- Removed active-client shutdown coverage — Deleted
redis_fixture_stops_gracefully_with_active_client_connectionand its supporting imports without adding replacement coverage.
⚠️ The Redis shutdown fix has lost its regression coverage
The deleted test was the only case that held a live Redis client connection open while stopping the supervised fixture. Existing Redis reconciliation tests do not exercise that condition, so reverting daemon_threads = True can restore the ten-second fallback without failing CI; this also contradicts the PR's stated goal of reducing runtime without reducing coverage.
Technical details
# The Redis shutdown fix has lost its regression coverage
## Affected sites
- `crates/daemon/src/managed_resources/tests.rs` — commit `26eb2ec` deletes `redis_fixture_stops_gracefully_with_active_client_connection`, which was added in `3188967` to cover the exact active-handler failure identified in the prior review.
- `crates/daemon/src/managed_resources/tests.rs:4914` — `RedisServer.daemon_threads = True` remains the load-bearing fix, but no remaining test distinguishes it from the default `False` behavior.
## Required outcome
- Preserve automated coverage that keeps a Redis fixture handler active during supervised shutdown and proves shutdown completes before the ten-second force-kill fallback.GPT Sol ｜ 𝕏
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26eb2ec390
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| --base-url https://artifacts.example.test | ||
|
|
||
| - name: Run tests | ||
| run: cargo nextest run --workspace --all-features --locked |
There was a problem hiding this comment.
In the CI workflow I inspected, this is now the final Rust validation step; the previous cargo doc --workspace --all-features --no-deps --locked with RUSTDOCFLAGS=-D warnings was deleted. I checked cargo doc --help, which describes it as building package documentation, so PRs that introduce rustdoc-only warnings such as broken intra-doc links can now merge because the remaining test/clippy steps do not build docs. Please keep this step or replace it with an equivalent cheaper rustdoc check if the goal is no coverage loss.
Useful? React with 👍 / 👎.
1 participant
Summary
shutdown()calls waited for the supervisor grace periodRoot cause
The cited PR job took 7m04s, including a 4m32s test step. The nextest override serialized a 254-test union representing roughly 95% of aggregate test execution. Several Redis/RustFS fixtures also called
socketserver.BaseServer.shutdown()from the same thread asserve_forever(), so they waited for the production ten-second grace/force-kill path. CI additionally rebuilt internal Rust docs, repeated recipe CLI work outside the test suite, and had no Rust build cache.Impact
The full application suite remains selected. Three consecutive final default-profile runs each passed all 1,004 selected tests, with nextest execution of:
This is local execution evidence only; the PR's GitHub Actions wall time should be evaluated from the remote run. Internal Rust docs no longer gate PRs by design, while recipe binary dispatch remains automatically covered by integration tests.
Validation
cargo fmt --all --checkcargo shearshellcheckactionlint .github/workflows/ci.ymlSummary by CodeRabbit
CI Improvements
Bug Fixes
Tests
pv-releasecommand-line flow directly.Documentation